Revamp content display API#2693
Conversation
dumptruckman
left a comment
There was a problem hiding this comment.
Some notes for now. I'll have to look again later. Heading to bed.
|
|
||
| private Collection<String> getListContents(@NotNull CommandSender sender) { | ||
| Player player = (sender instanceof Player) ? (Player) sender : null; | ||
| private ContentParser newWorldListContentParser() { |
There was a problem hiding this comment.
I think that this might be more readable as a separate class. It could just be a private static one in this class maybe?
There was a problem hiding this comment.
yea can change that. Probably use a singleton instance?
There was a problem hiding this comment.
hmm dont really want do use static class so I don't have to pass in the plugin instance to the private class, so current just doing something like private class WorldListContentParser implements ContentParser { and then .addContentParser(new WorldListContentParser());
There was a problem hiding this comment.
What's wrong with passing the plugin instance into the new class's constructor?
There was a problem hiding this comment.
tbh nothing wrong, but then I can't really use a singleton instance anyways, as not a good idea to store plugin objects in static like this right? Like if a new instance plugin instance is created, it may cause issues? So I just decided to make it inner class nonstatic so it uses the instance from the parent class.
| * @param sender The target which the content will be displayed to. | ||
| * @param content The content to display. | ||
| */ | ||
| void parse(@NotNull CommandSender sender, @NotNull List<String> content); |
There was a problem hiding this comment.
I think this might be better if it returns the content list instead of is given one to add to. I think this is just a gut feeling and maybe isn't entirely right but I feel like a list you're given to modify smells a bit more than just returning a list.
There was a problem hiding this comment.
The reason why this doesn't just return a list of strings so we only need one common list. Instead of each parsing creating a new list as then adding it to another common list... which is potentially is a lot of lists.
For whether if we delete/modify existing entries from other parsers, either we allow it as a feature, or just instead pass a wrapper around the List that only allow adding of elements.
But all these won't be needed if we decide to only allow one content parser.
| public interface ContentFilter { | ||
| /** | ||
| * Checks if a particular string should be displayed. | ||
| * | ||
| * @param value String to check on. | ||
| * @return True if should be display, false otherwise. | ||
| */ | ||
| boolean checkMatch(String value); | ||
|
|
||
| /** | ||
| * Gets whether content needs to be filtered by this filter. | ||
| * | ||
| * @return True if content should be filtered, false otherwise. | ||
| */ | ||
| boolean needToFilter(); | ||
| } |
There was a problem hiding this comment.
First glance, this seems like it could easily be a single method, functional interface. If the need to filter is false, then the checkMatch should just return true. In that case, you could also just use the built in Predicate interface.
There was a problem hiding this comment.
Yea, then reason I d have the second method, is so that there is no need to loop through the content to filter is there nothing to filter. And when displaying to user, it output whether or not filter was applied to the content.
There was a problem hiding this comment.
Example use cases:
// Only send output of filter string used if needed.
if (filter.needToFilter()) {
sender.sendMessage(String.format("%s[Filter '%s']", ChatColor.GRAY, filter));
}// Only loop to filter if needed, to save performance
if (filter.needToFilter()) {
return content.stream().filter(filter::checkMatch).collect(Collectors.toList());
}
return content;* Use singleton pattern for DefaultContentFilter with getInstance method. * Have a default SendHandler. * Don't need streams for small dataset. * Private WorldListContentParser class to improve readability.
|
implemented in #2824 |
improve based on @dumptruckman comments
content display API changes
Other additions